[GOBBLIN-2135] Cache Gobblin YARN application jars #4030

Will-Lo · 2024-08-16T02:45:21Z

Dear Gobblin maintainers,

Please accept this PR. I understand that it will not be reviewed until I have checked off all the steps below!

JIRA

My PR addresses the following Gobblin JIRA issues and references them in the PR title. For example, "[GOBBLIN-XXX] My Gobblin PR"
- https://issues.apache.org/jira/browse/GOBBLIN-2135

Description

Here are some details about my PR, including screenshots (if applicable):
Gobblin YARN Application Launcher lacks some functionality used in MRJobLauncher. One of the biggest gaps in feature parity is the absence of jar caching, where MRJobLauncher creates a monthly cache that is automatically cleaned up by subsequent executions performed 2 months in advance.

YARN/MR requires uploading jars to HDFS, this step can be quite slow (~15 mins for a sizeable job to get all the jars), and given that many jobs do share the same jars, it makes sense to cache them together and only provide YARN the shared path.

We also want to ensure that SNAPSHOT jars are other files are not uploaded to a cache, since they are not immutable unlike jar versions on Artifactory.

This PR implements jar caching through 2 configurations:

gobblin.yarn.jar.cache.enabled
gobblin.yarn.jar.cache.dir

Where if gobblin.yarn.jar.cache.enabled=true, then it will look for the directory defined in gobblin.yarn.jar.cache.dir. It is expected that snapshot jars and other files are stored in some directory that is unique to the execution so that those jars will not be shared across other concurrent executions, only jars stored in the jar cache will be.

Tests

My PR adds the following unit tests OR does not need testing for this extremely good reason:
It is tested that this saves approximately 10 minutes of bootstrap time per job.

Commits

My commits all reference JIRA issues in their subject lines, and I have squashed multiple commits if they address the same issue. In addition, my commits follow the guidelines from "How to write a good git commit message":
1. Subject is separated from body by a blank line
2. Subject is limited to 50 characters
3. Subject does not end with a period
4. Subject uses the imperative mood ("add", not "adding")
5. Body wraps at 72 characters
6. Body explains "what" and "why", not "how"

…epeatedly upload jars and files to hdfs

gobblin-runtime/src/main/java/org/apache/gobblin/runtime/mapreduce/MRJobLauncher.java

phet · 2024-08-29T00:12:44Z

gobblin-runtime/src/main/java/org/apache/gobblin/runtime/mapreduce/MRJobLauncher.java

          LOG.info(String.format("Adding %s to classpath", destJarFile));
          DistributedCache.addFileToClassPath(destJarFile, conf, this.fs);
+        } else {
+          LOG.error("Failed to upload jar file: " + status.getPath());


I don't find the prior code throwing an error...

nonetheless, should everything continue on w/ just some error logs?

shouldn't we instead fail the overall job because presumably necessary jars won't be there?

Possibly we should throw an error, but I think given that another job could be uploading the same jars though it might be better to let the job attempt to try and run, if that job fails it should be emitting the failed event anyways.

gobblin-temporal/src/main/java/org/apache/gobblin/temporal/yarn/YarnService.java

gobblin-yarn/src/main/java/org/apache/gobblin/yarn/YarnHelixUtils.java

gobblin-utility/src/main/java/org/apache/gobblin/util/filesystem/HdfsJarUploadUtils.java

phet · 2024-09-05T06:01:42Z

gobblin-utility/src/main/java/org/apache/gobblin/util/filesystem/HdfsJarUploadUtils.java

+   * @return
+   * @throws IOException
+   */
+  public static boolean uploadJarToHdfs(FileSystem fs, FileStatus localJar, int jarFileMaximumRetry, Path destJarFile) throws IOException {


jarFileMaximumRetry => simply maxAttempts?

phet · 2024-09-05T06:07:22Z

gobblin-temporal/src/main/java/org/apache/gobblin/temporal/yarn/YarnService.java

+    }
+    if (this.jarCacheEnabled) {
+      addContainerLocalResources(new Path(jarCacheDir, GobblinYarnConfigurationKeys.LIB_JARS_DIR_NAME), resourceMap);
+      if (this.config.hasPath(GobblinYarnConfigurationKeys.CONTAINER_FILES_LOCAL_KEY)) {


I don't quite understand this key. you're checking it here in two different conditionals, but in neither one do you actually use (or even check to see) what value it holds

Oops should have used gobblin.yarn.container.jars key

phet · 2024-09-05T06:09:32Z

gobblin-yarn/src/main/java/org/apache/gobblin/yarn/GobblinYarnAppLauncher.java

-    this.appLauncherMode = ConfigUtils.getString(config, GOBBLIN_YARN_APP_LAUNCHER_MODE, DEFAULT_GOBBLIN_YARN_APP_LAUNCHER_MODE);
-
+    this.appLauncherMode = ConfigUtils.getString(this.config, GOBBLIN_YARN_APP_LAUNCHER_MODE, DEFAULT_GOBBLIN_YARN_APP_LAUNCHER_MODE);
+    this.jarCacheEnabled = ConfigUtils.getBoolean(config, GobblinYarnConfigurationKeys.JAR_CACHE_ENABLED, GobblinYarnConfigurationKeys.JAR_CACHE_ENABLED_DEFAULT);


NBD, but you just updated the two above to be this.config, but only use config here :)

gobblin-yarn/src/main/java/org/apache/gobblin/yarn/GobblinYarnAppLauncher.java

phet · 2024-09-05T06:20:11Z

gobblin-yarn/src/main/java/org/apache/gobblin/yarn/GobblinYarnAppLauncher.java

-      }
-      if (resourceMap.isPresent()) {
-        YarnHelixUtils.addFileAsLocalResource(this.fs, destFilePath, LocalResourceType.FILE, resourceMap.get());
+        LOGGER.warn("Failed to upload jar file {} to HDFS", srcFilePath);


again, should this be an actual failure, not merely logging?

...or do we believe there are times when it's actually OK to continue?

I think the concern is valid but currently trying to get parity with MR job launcher, I think since there could always be concurrent executions adding the jars instead so it can be worthwhile to just attempt the job, it will fail loudly if the jars weren't uploaded properly anyways.

I guess that's fine to start. how about a

// TODO: decide whether to fail-fast here, given the job may be unable to run w/o it

phet · 2024-09-12T15:27:46Z

gobblin-yarn/src/main/java/org/apache/gobblin/yarn/GobblinYarnAppLauncher.java

+    if (this.jarCacheEnabled) {
+      Path jarCachePath = YarnHelixUtils.calculateJarCachePath(this.config);
+      // Retain at least the current and last month's jars to handle executions running for ~30 days max
+      boolean cleanedSuccessfully = YarnHelixUtils.retainKLatestJarCachePaths(jarCachePath.getParent(), 2, this.fs);


does this run before or after caching jars? e.g. do we save only two prior AND THEN potentially add one more or we've added any new one already prior to retention paring it down to two?

It runs after caching the jars. But it uses a consistent YARN_APPLICATION_LAUNCHER_START_TIME_KEY in the job so no matter how many times we look at the cache path it's only creating one path at most, and that path would be the ones where the jars are being uploaded.

would be great to add this info to a code comment here (in the caller, since it's the combination of the two being used, so less effective in either's javadoc). the crux is that retention of K=2 won't save us in cases where K=3 might exceed any FS quotas.

gobblin-yarn/src/main/java/org/apache/gobblin/yarn/YarnHelixUtils.java

phet · 2024-09-12T15:30:16Z

gobblin-yarn/src/main/java/org/apache/gobblin/yarn/YarnHelixUtils.java

+    if (jarDirs.size() > k) {
+      return fs.delete(jarDirs.get(0).getPath(), true);


since this is not a loop, it seems it would delete at most one dir even if there are more than 1 more than k. is that ok? if so, document in javadoc

Changed it to use a loop, for consistency with naming convention.

codecov-commenter · 2024-09-12T17:57:55Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 55.31%. Comparing base (e501b62) to head (72eb403).
Report is 23 commits behind head on master.

Additional details and impacted files

@@             Coverage Diff              @@
##             master    #4030      +/-   ##
============================================
+ Coverage     45.86%   55.31%   +9.45%     
+ Complexity     3257     1582    -1675     
============================================
  Files           707      307     -400     
  Lines         27865    10580   -17285     
  Branches       2796     1069    -1727     
============================================
- Hits          12779     5852    -6927     
+ Misses        14008     4223    -9785     
+ Partials       1078      505     -573

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

phet · 2024-09-16T23:55:59Z

gobblin-utility/src/main/java/org/apache/gobblin/util/filesystem/HdfsJarUploadUtils.java

+   * @param localJarPath
   * @param unsharedJarsDir
   * @param jarCacheDir
   * @return
   * @throws IOException
   */
-  public static Path calculateDestJarFile(FileSystem fs, FileStatus localJar, Path unsharedJarsDir, Path jarCacheDir) throws IOException {
-    Path uploadDir = localJar.getPath().getName().contains("SNAPSHOT") ? unsharedJarsDir : jarCacheDir;
-    Path destJarFile = new Path(fs.makeQualified(uploadDir), localJar.getPath().getName());
+  public static Path calculateDestJarFilePath(FileSystem fs, String localJarPath, Path unsharedJarsDir, Path jarCacheDir) throws IOException {
+    Path uploadDir = localJarPath.contains("SNAPSHOT") ? unsharedJarsDir : jarCacheDir;


seeing this invoked, localJarPath looks more like localJarBasename, not the (full) path.

(or simply jarName)

phet · 2024-09-17T00:14:34Z

gobblin-yarn/src/main/java/org/apache/gobblin/yarn/YarnHelixUtils.java

-      return fs.delete(jarDirs.get(0).getPath(), true);
+    boolean deletesSuccessful = true;
+    for (int i = 0; i < jarDirs.size() - k; i++) {
+      deletesSuccessful = deletesSuccessful && fs.delete(jarDirs.get(i).getPath(), true);


phet · 2024-09-17T00:19:41Z

gobblin-yarn/src/main/java/org/apache/gobblin/yarn/GobblinYarnAppLauncher.java

+    if (this.jarCacheEnabled) {
+      Path jarCachePath = YarnHelixUtils.calculateJarCachePath(this.config);
+      // Retain at least the current and last month's jars to handle executions running for ~30 days max
+      boolean cleanedSuccessfully = YarnHelixUtils.retainKLatestJarCachePaths(jarCachePath.getParent(), 2, this.fs);


would be great to add this info to a code comment here (in the caller, since it's the combination of the two being used, so less effective in either's javadoc). the crux is that retention of K=2 won't save us in cases where K=3 might exceed any FS quotas.

phet

Great work william - I can't wait to get this in and streamline every startup!

Will-Lo added 5 commits August 15, 2024 22:42

Implement caching for gobblin yarn app launcher so that jobs do not r…

7b5706e

…epeatedly upload jars and files to hdfs

Add javadoc for util class

cca5c0a

Cleanup

1743bb3

Avoid adding container jars if unconfigured

ee790d1

Fix checkstyle

9a2f8fb

Will-Lo changed the title ~~Cache temporal jars~~ [GOBBLIN-2135] Cache Gobblin YARN application jars Aug 19, 2024

homatthew reviewed Aug 22, 2024

View reviewed changes

gobblin-runtime/src/main/java/org/apache/gobblin/runtime/mapreduce/MRJobLauncher.java Show resolved Hide resolved

phet reviewed Sep 5, 2024

View reviewed changes

Will-Lo added 2 commits September 11, 2024 16:15

Address review

f290629

Add tests

614e4bc

phet reviewed Sep 12, 2024

View reviewed changes

Address reviews

72eb403

Cleanup

88fdddc

Will-Lo force-pushed the cache-temporal-jars branch from 23a9b21 to 88fdddc Compare September 16, 2024 17:47

phet reviewed Sep 17, 2024

View reviewed changes

Address review, improve comments/javadoc

3aa8a08

phet approved these changes Sep 17, 2024

View reviewed changes

Will-Lo merged commit a07a4f1 into apache:master Sep 17, 2024

Will-Lo deleted the cache-temporal-jars branch September 17, 2024 21:15

		if (jarDirs.size() > k) {
		return fs.delete(jarDirs.get(0).getPath(), true);

[GOBBLIN-2135] Cache Gobblin YARN application jars #4030

[GOBBLIN-2135] Cache Gobblin YARN application jars #4030

Uh oh!

Conversation

Will-Lo commented Aug 16, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

JIRA

Description

Tests

Commits

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

phet Sep 12, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov-commenter commented Sep 12, 2024

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

phet left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Will-Lo commented Aug 16, 2024 •

edited

Loading

phet Sep 12, 2024 •

edited

Loading